feat(14.1.0): manifest-hash check stops crying wolf and starts checking files (#224); fedcode v3 embeds owned nodes (#269) - #270
Conversation
…ng files (#224); fedcode v3 embeds owned nodes (#269) #224 -- a false alarm that also disabled the real alarm. Three manifest_hash constructions exist in this repo: concatenated per-file hashes (file_integrity), concatenated per-FUNCTION hashes (ciris-manifest-tool), and sha256 over serde-JSON bytes (ciris-build-tool). A manifest produced by one and checked by another can never match. The old code recomputed exactly one, SELF-DIAGNOSED the benign cause in its own warning, and still reported a tampering-shaped ERROR -- 14 of them in a 4h window on datum, one per Discord reconnect. The half nobody reported is worse: check_full then returned early with files_checked: 0, so the per-file hashes -- the control that can actually detect tampering -- were never checked. Same symptom #176 reports as "L4 file integrity will be skipped". Now it TRIES the other construction instead of guessing, returning ManifestHashCheck::{VerifiedConcatenated, VerifiedJson, Unrecognized}. Unrecognized is deliberately not called "mismatch": with only files + manifest_hash in hand, a fourth construction is INDISTINGUISHABLE from tampering, so asserting either manufactures a verdict from a measurement we cannot make (MISSION.md 1.4 -- the distinction v13.3.0 drew for revocation). CONTRACT CHANGE consumers must know: an unrecognized hash no longer flips integrity_valid; it is surfaced on its own field and the per-file check runs. Altering manifest_hash alone achieves nothing anyway -- the per-file hashes are untouched, and an attacker who altered those would recompute it, which this check never caught either. It is a self-consistency checksum, not an authentication. A malformed manifest is still a hard failure. #269 -- a user code that MAY embed its owned nodes. CC 5.4.6 names the population v1 cannot serve: phone-class peers that cannot hold the full directory -- first contact, a QR across a table, an air-gapped hand-off. NOTE THE VERSION. The proposal was called "fedcode v2" and the wire format has been at v2 since the kind-tagged code shipped, so this mints v3 (CIRIS-V3-). Minting it as v2 would have collided with a live encoding. Additive and free: a code with no nodes still emits BYTE-IDENTICAL v2, so nothing already issued moves and the MAY costs nothing. The safety property is the point. OwnedNode carries the node's TRANSPORT Ed25519, never the owner's federation key. CIRISServer#335 is what that confusion cost: nodes primed the canonical at 1fc232535a... while it served on 81cabcf78a..., every node reported knows_peer=true, provenance=Rooted, primed=1, refused=0, and ZERO traces arrived -- then the false rooting PREVENTED recovery, because a node that believes it knows a peer never learns the real address. What made it survive review is that transport and federation share the Ed25519 half, so the derivation looks sound; sharing a key does not make a base hash and a named hash the same address. Refused at BOTH encoder and decoder, because a code minted by another implementation is exactly the case an encoder cannot police. Only a user code may carry nodes (a group's destinations are group-scoped material a code must not carry at all, CC 5.4.6), capped at 16 so a code stays scannable. 11 new tests. 1396 workspace green, clippy clean, doc clean, guards pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FZPhbXWTzykcbotEX9BN2p
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79c5c46478
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let owned_nodes = if version == FEDCODE_VERSION_V3 { | ||
| let count = usize::from(*payload.get(offset).ok_or_else(trunc)?); |
There was a problem hiding this comment.
A foreign implementation can construct a CRC-valid v3 payload whose kind is Agent, Family, or Community, and this branch will parse and return its embedded nodes even though build_payload explicitly forbids that combination. Since scanned fedcodes are untrusted inputs, consumers can consequently receive a FedCode that violates the advertised user-only invariant and may treat attacker-supplied nodes as owned by a non-user entity; apply the same kind == FedKind::User validation on decode.
Useful? React with 👍 / 👎.
| if let Ok(json) = serde_json::to_vec(&manifest.files) { | ||
| if super::constant_time_eq( | ||
| hex::encode(Sha256::digest(&json)).as_bytes(), | ||
| stored.as_bytes(), | ||
| ) { | ||
| return Some(ManifestHashCheck::VerifiedJson); |
There was a problem hiding this comment.
For actual manifests registered by ciris-build-tool, this comparison can never produce VerifiedJson: src/ciris-build-tool/src/register.rs:601-605 hashes the serialized wrapper {"files": files}, while this code hashes only the bare file map. Thus every manifest from the producer this branch claims to recognize is still classified and logged as Unrecognized; serialize the same wrapper shape here and add a parity test against per_target_file_manifest_payload.
Useful? React with 👍 / 👎.
| /// Which construction reproduced the stored `manifest_hash`, if any | ||
| /// (CIRISVerify#224). [`ManifestHashCheck::Unrecognized`] is a | ||
| /// measurement, not a tampering verdict — read `per_file_results` for | ||
| /// that. | ||
| #[serde(default = "manifest_hash_check_default")] | ||
| pub manifest_hash_check: ManifestHashCheck, |
There was a problem hiding this comment.
When a production unified or CLI check encounters an unrecognized manifest hash, integrity_valid now remains true, but this newly added status is not copied by From<FileIntegrityResult> for FileCheckSummary in unified.rs and is also omitted from the CLI JSON assembled in ciris_verify.rs. Those primary callers therefore report an ordinary pass with no indication that the aggregate checksum was unrecognized, contrary to the stated contract that the condition is surfaced; add the field to those output shapes and conversions.
Useful? React with 👍 / 👎.
| if manifest.manifest_hash.is_empty() { | ||
| tracing::error!("verify_manifest_integrity: FAILED - manifest_hash is empty!"); | ||
| return false; | ||
| tracing::error!("manifest integrity: manifest_hash is empty"); | ||
| return None; | ||
| } | ||
|
|
||
| let mut hasher = Sha256::new(); | ||
| // Hash all file hashes in sorted order (BTreeMap is already sorted) | ||
| let mut hash_count = 0; | ||
| for (path, hash) in manifest.files.iter() { | ||
| hasher.update(hash.as_bytes()); | ||
| hash_count += 1; | ||
| // Log first 3 entries for debugging | ||
| if hash_count <= 3 { | ||
| tracing::debug!( | ||
| "verify_manifest_integrity: hashing file #{}: path='{}', hash='{}'", | ||
| hash_count, | ||
| path, | ||
| &hash[..std::cmp::min(16, hash.len())] | ||
| ); | ||
| } | ||
| } | ||
| let computed = hex::encode(hasher.finalize()); | ||
|
|
||
| // Strip "sha256:" prefix from stored hash if present | ||
| let stored_clean = manifest | ||
| let stored = manifest | ||
| .manifest_hash | ||
| .strip_prefix("sha256:") | ||
| .unwrap_or(&manifest.manifest_hash); |
There was a problem hiding this comment.
A manifest whose hash is "sha256:" bypasses this empty-value check because the raw field is nonempty, but stripping the prefix produces an empty digest; the function then returns Some(Unrecognized) and all three check modes can report integrity_valid: true when the files match. The same happens for arbitrary non-hex or wrong-length values, so malformed hashes are treated as unknown constructions rather than the documented hard failure; validate the stripped digest as exactly 64 hexadecimal characters before continuing.
Useful? React with 👍 / 👎.
| if fc.owned_nodes.len() > MAX_OWNED_NODES { | ||
| return Err(FedCodeError::Malformed(format!( | ||
| "at most {MAX_OWNED_NODES} owned nodes, got {}", | ||
| fc.owned_nodes.len() | ||
| ))); | ||
| } | ||
| out.push(fc.owned_nodes.len() as u8); |
There was a problem hiding this comment.
The 16-node limit does not ensure that the resulting code remains renderable as a QR because each accepted node may carry a 255-byte key ID plus its 32-byte key, producing more than 4.6 KB of binary payload and over 7 KB after base32 encoding. Such a FedCode is accepted by encode, but the CLI's QrCode::new path returns None for an oversized code, defeating the advertised scannable handoff; enforce a total encoded-size budget or reduce field/count limits to the QR renderer's capacity.
Useful? React with 👍 / 👎.
| /// **MAY** carry the owner's nodes, so a contact resolves with no | ||
| /// directory (CIRISVerify#269) — first contact, a QR across a table, an | ||
| /// air-gapped hand-off, a fresh install. | ||
| /// | ||
| /// Empty is valid and is the default: a code with no nodes degrades to the | ||
| /// v1/v2 directory path, and **encodes byte-identically to a v2 code**, so | ||
| /// nothing already issued changes. Only a non-empty list emits v3. | ||
| /// | ||
| /// Scope boundary: these are **lightnet** facts — federation-scope | ||
| /// identity that already announces publicly and carries no anonymity | ||
| /// claim. A code MUST NOT carry group-scoped material, whose destinations | ||
| /// are derived from `cached directory + per-group HKDF` and may not be | ||
| /// emitted at all (CC 5.4.6, ruled in CIRISConstitution#91). | ||
| pub owned_nodes: Vec<OwnedNode>, |
There was a problem hiding this comment.
Although decoding a v3 code now populates this field, emit_fedcode still omits it from both the fedcode show --json object and the human-readable display. A CLI consumer therefore cannot obtain the embedded node IDs or transport keys needed for the advertised directory-free resolution and only gets the opaque code echoed back; serialize and display owned_nodes alongside the existing decoded fields.
Useful? React with 👍 / 👎.
| /// Binary-format version for a code that MAY embed the owner's nodes | ||
| /// (CIRISVerify#269). | ||
| /// | ||
| /// **Note the number.** The issue proposing this called it "fedcode v2", but | ||
| /// the wire format has been at v2 since the kind-tagged code shipped — so the | ||
| /// node-carrying format is **v3**. Minting it as "v2" would have collided with | ||
| /// a live encoding. | ||
| pub const FEDCODE_VERSION_V3: u8 = 0x03; | ||
|
|
||
| const PREFIX_V3: &str = "CIRIS-V3-"; |
There was a problem hiding this comment.
This introduces a new shared binary version and payload tail, but FSD/FSD-003_FEDERATION_IDENTITY_CODES.md:82-126 still defines only v2, lists 0x02 as the version, and says cross-implementations are judged byte-for-byte against that layout. CIRISServer, CIRISAgent, and other consumers therefore have no authoritative description from which to implement interoperable v3 encoding or decoding; update the wire layout, prefix rules, validation constraints, and cross-implementation obligations with this change.
Useful? React with 👍 / 👎.
1 participant
Closes #224 and #269 — the two the backlog pass surfaced as most actionable.
#224 — a false alarm that also disabled the real alarm
Three
manifest_hashconstructions exist in this repository:file_integrityciris-manifest-toolciris-build-toolA manifest produced by one and checked by another can never match. The old code recomputed exactly one, self-diagnosed the benign cause in its own warning, and still reported a tampering-shaped
ERROR— 14 in a 4h window ondatum, one per Discord reconnect.The half nobody reported is worse.
check_fullthen returned early withfiles_checked: 0— so the per-file hashes, the control that can actually detect tampering, were never checked. That is the same symptom #176 reports as "L4 file integrity will be skipped".Now it tries the other construction instead of guessing:
Unrecognizedis deliberately not called "mismatch". With onlyfilesandmanifest_hashin hand, a fourth construction is indistinguishable from tampering — so asserting either manufactures a verdict from a measurement we cannot make (MISSION.md§1.4, the same distinction v13.3.0 drew for revocation).Contract change worth reading
An unrecognized hash no longer flips
integrity_valid; it is surfaced on its own field and the per-file check runs.I rewrote
test_manifest_tamper_detectionrather than making it pass, because its contract was wrong in both directions. Note what alteringmanifest_hashalone actually achieves: nothing. The per-file hashes are untouched, so files are still checked against real values — and an attacker who altered those would simply recomputemanifest_hashto match, which this check never caught either. It is a self-consistency checksum, not an authentication.A structurally malformed manifest (no files, empty hash) is still a hard failure — that is not a disagreement about algorithms.
#269 — a user code that MAY embed its owned nodes
CC 5.4.6 names the population v1 cannot serve: "phone-class peers that cannot hold the full directory" — first contact, a QR across a table, an air-gapped hand-off.
Note the version number
The proposal is titled "fedcode v2" — and the wire format has been at v2 since the kind-tagged code shipped. So this mints v3 (
CIRIS-V3-). Minting it as "v2" would have collided with a live encoding.Additive and free: a code with no nodes still emits byte-identical v2, so nothing already issued moves and the
MAYgenuinely costs nothing.The safety property is the point
OwnedNodecarries the node's transport Ed25519 — never the owner's federation key.CIRISServer#335 is what that confusion cost in production: nodes primed the canonical at
1fc232535a…while it served on81cabcf78a…. Every node reportedknows_peer=true, provenance=Rooted, primed=1, refused=0, and zero traces arrived — then the false rooting prevented recovery, because a node that believes it knows a peer never learns the real address.What made it survive review is that transport and federation share the Ed25519 half, so the derivation looks sound. Sharing a key does not make a base hash and a named hash the same address.
Refused at both encoder and decoder — a code minted by another implementation is exactly the case an encoder cannot police. Only a
usercode may carry nodes (a group's destinations are group-scoped material a code must not carry at all, CC 5.4.6), capped at 16 so a code stays scannable.11 new tests. 1396 workspace green, clippy clean, doc clean, all guards + self-test pass.
🤖 Generated with Claude Code
https://claude.ai/code/session_01FZPhbXWTzykcbotEX9BN2p